[slice] Rename states + add timeout #810

pajakd · 2025-11-05T12:47:37Z

Description

Issue

Testing

mbobrovskyi · 2025-11-07T10:23:51Z

slice/api/v1alpha1/slice_types.go

 )

 // NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.
+type Type string


Suggested change

type Type string

type AcceleratorType string

Maybe like this?

For the slice API, this is just copying the canonical version of this API (exists in some doc), and we need to match that.

mbobrovskyi · 2025-11-07T10:24:36Z

slice/api/v1alpha1/slice_types.go

 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )

 // NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.


It looks like this note related to Type. Can we add some space between to avoid confusion?

mbobrovskyi · 2025-11-07T10:25:09Z

slice/api/v1alpha1/slice_types.go

+	// Type specifies the type of accelerator used in this slice, e.g., "v6e", "tpu-v7x".
 	// +kubebuilder:validation:Immutable
-	Type string `json:"type"`
+	// +kubebuilder:validation:Enum=v6e;tpu-v7x


Can we move this validation to type?

mbobrovskyi · 2025-11-07T10:25:23Z

slice/api/v1alpha1/slice_types.go

 )

 // NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.
+type Type string


Like this

Suggested change

type Type string

// +kubebuilder:validation:Enum=v6e;tpu-v7x

type Type string

slice/internal/controller/workload_controller.go

mbobrovskyi · 2025-11-07T10:36:13Z

slice/internal/core/constants.go

 	TPUSliceHealthNodeSelectorValue = "true"

 	AcceleratorTpu7x = "tpu-v7x"
+


Suggested change

)

const(

mbobrovskyi · 2025-11-07T10:37:26Z

slice/internal/core/constants.go


 package core

+type MMIGHealthStatus string


I would also move it to core.go.

mbobrovskyi · 2025-11-07T10:38:08Z

slice/internal/core/slice.go

-func Deformed(slice *v1alpha1.Slice) bool {
-	return meta.IsStatusConditionTrue(slice.Status.Conditions, string(v1alpha1.Deformed))
+func IsStale(slice *v1alpha1.Slice) bool {
+	cond := meta.FindStatusCondition(slice.Status.Conditions, string(v1alpha1.SliceStateConditionType))


Suggested change

cond := meta.FindStatusCondition(slice.Status.Conditions, string(v1alpha1.SliceStateConditionType))

cond := meta.FindStatusCondition(slice.Status.Conditions, v1alpha1.SliceStateConditionType)

mbobrovskyi · 2025-11-07T10:38:24Z

slice/internal/core/slice.go

+}
+
+func Deactivating(slice *v1alpha1.Slice) bool {
+	cond := meta.FindStatusCondition(slice.Status.Conditions, string(v1alpha1.SliceStateConditionType))


Suggested change

cond := meta.FindStatusCondition(slice.Status.Conditions, string(v1alpha1.SliceStateConditionType))

cond := meta.FindStatusCondition(slice.Status.Conditions, v1alpha1.SliceStateConditionType)

slice/internal/core/slice.go

mbobrovskyi · 2025-11-07T10:40:07Z

slice/internal/core/slice.go

+	return cond != nil && cond.Status == metav1.ConditionFalse && time.Since(cond.LastTransitionTime.Time) >= ActivationTimeout
+}
+
+func Deactivating(slice *v1alpha1.Slice) bool {


I think this is unused.

Right, removed.

mbobrovskyi · 2025-11-07T10:40:55Z

slice/internal/util/testing/wrappers.go

 	return s
 }

 func (s *SliceWrapper) Type(Type string) *SliceWrapper {


Suggested change

func (s *SliceWrapper) Type(Type string) *SliceWrapper {

func (s *SliceWrapper) Type(acceleratorType string) *SliceWrapper {

Or

Suggested change

func (s *SliceWrapper) Type(Type string) *SliceWrapper {

func (s *SliceWrapper) Type(t string) *SliceWrapper {

mbobrovskyi · 2025-11-07T10:41:58Z

slice/internal/util/testing/wrappers.go

+func (s *SliceWrapper) Active() *SliceWrapper {
 	cond := metav1.Condition{
-		Type:               string(v1alpha1.Ready),
+		Type:               string(v1alpha1.SliceStateConditionType),


Suggested change

Type: string(v1alpha1.SliceStateConditionType),

Type: v1alpha1.SliceStateConditionType,

mbobrovskyi · 2025-11-07T10:42:15Z

slice/internal/util/testing/wrappers.go

 	cond := metav1.Condition{
-		Type:               string(v1alpha1.Forming),
-		Status:             metav1.ConditionTrue,
+		Type:               string(v1alpha1.SliceStateConditionType),


Suggested change

Type: string(v1alpha1.SliceStateConditionType),

Type: v1alpha1.SliceStateConditionType,

mbobrovskyi · 2025-11-07T10:42:25Z

slice/internal/util/testing/wrappers.go

+func (s *SliceWrapper) Degraded() *SliceWrapper {
 	cond := metav1.Condition{
-		Type:               string(v1alpha1.Deformed),
+		Type:               string(v1alpha1.SliceStateConditionType),


Suggested change

Type: string(v1alpha1.SliceStateConditionType),

Type: v1alpha1.SliceStateConditionType,

mbobrovskyi · 2025-11-07T10:43:01Z

slice/internal/util/testing/wrappers.go

 	cond := metav1.Condition{
-		Type:               string(v1alpha1.Degraded),
-		Status:             metav1.ConditionTrue,
+		Type:               string(v1alpha1.SliceStateConditionType),


Suggested change

Type: string(v1alpha1.SliceStateConditionType),

Type: v1alpha1.SliceStateConditionType,

mbobrovskyi · 2025-11-07T10:43:28Z

slice/internal/util/testing/wrappers.go

-		Status:             metav1.ConditionTrue,
-		LastTransitionTime: metav1.Now(),
-		Reason:             "ByTest",
+		Type:               string(v1alpha1.SliceStateConditionType),


Suggested change

Type: string(v1alpha1.SliceStateConditionType),

Type: v1alpha1.SliceStateConditionType,

slice/internal/util/testing/wrappers.go

slice/internal/core/slice.go

mbobrovskyi · 2025-11-07T10:53:39Z

slice/internal/core/constants.go

 	TPUSliceHealthNodeSelectorKey   = "cloud.google.com/gke-tpu-slice-4x4x4-health"
 	TPUSliceHealthNodeSelectorValue = "true"

 	AcceleratorTpu7x = "tpu-v7x"


I think we have it in API, right? Can we remove it?

mbobrovskyi · 2025-11-07T10:55:20Z

slice/api/v1alpha1/slice_types.go

+type Type string
+
+const (
+	TypeV6e   Type = "v6e"


Suggested change

TypeV6e Type = "v6e"

AcceleratorTypeV6e Type = "v6e"

mbobrovskyi · 2025-11-07T10:55:26Z

slice/api/v1alpha1/slice_types.go

+
+const (
+	TypeV6e   Type = "v6e"
+	TypeTpu7x Type = "tpu-v7x"


Suggested change

TypeTpu7x Type = "tpu-v7x"

AcceleratorTypeTpu7x Type = "tpu-v7x"

mbobrovskyi · 2025-11-07T10:56:36Z

slice/api/v1alpha1/slice_types.go

+type Type string
+
+const (
+	TypeV6e   Type = "v6e"


Should we relax validation for it in IsValidTPUAccelerator()?

We will need to implement logic supporting v6 before relaxing this.

mbobrovskyi · 2025-11-07T10:57:56Z

slice/api/v1alpha1/slice_types.go

 // SliceSpec defines the desired state of Slice.
 type SliceSpec struct {
-	// AcceleratorType specifies the type of accelerator used in this slice.
+	// Type specifies the type of accelerator used in this slice, e.g., "v6e", "tpu-v7x".


Suggested change

// Type specifies the type of accelerator used in this slice, e.g., "v6e", "tpu-v7x".

// type specifies the type of accelerator used in this slice, e.g., "v6e", "tpu-v7x".

mbobrovskyi · 2025-11-07T10:58:09Z

slice/api/v1alpha1/slice_types.go

+	// +kubebuilder:validation:Enum=v6e;tpu-v7x
+	Type Type `json:"type"`

 	// Topology represents the network topology of the slice.


Suggested change

// Topology represents the network topology of the slice.

// topology represents the network topology of the slice.

mbobrovskyi · 2025-11-07T10:58:28Z

slice/api/v1alpha1/slice_types.go

+	// +kubebuilder:validation:Pattern=^\d+x\d+(x\d+)?$
 	Topology string `json:"topology"`

 	// Partition Ids denotes the set of partitions to use to form a slice


Suggested change

// Partition Ids denotes the set of partitions to use to form a slice

// partitionIds denotes the set of partitions to use to form a slice

Also the same for status.

Co-authored-by: Mykhailo Bobrovskyi <[email protected]>

PBundyra · 2025-11-07T12:47:43Z

Can we separate API changes and timeout logic into two PRs?

gabesaba

Looks good overall. Only one significant comment: I propose keeping the smaller internal notion of slice state separate from the MMIGHealthState.

I can try to give a LGTM from KubeCon, once you address comments. If not, @PBundyra or @mbobrovskyi can

gabesaba · 2025-11-07T21:56:01Z

slice/api/v1alpha1/slice_types.go

+type Type string
+
+const (
+	TypeV6e   Type = "v6e"


We will need to implement logic supporting v6 before relaxing this.

gabesaba · 2025-11-07T21:57:09Z

slice/api/v1alpha1/slice_types.go

 )

 // NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.
+type Type string


For the slice API, this is just copying the canonical version of this API (exists in some doc), and we need to match that.

gabesaba · 2025-11-07T22:35:06Z

slice/internal/core/core.go

+var SliceStates = []MMIGHealthStatus{
+	MMIGHealthStatusActivating, MMIGHealthStatusActive, MMIGHealthStatusActiveDegraded,
+	MMIGHealthStatusDeactivating, MMIGHealthStatusFailed, MMIGHealthStatusIncomplete,
+	MMIGHealthStatusUnknown,


I think we should keep separate the notion of slice state from MMIGHealthStatus. This fewer number of states will be more managable in the controller. WDYT?

We would have a function of Ready condtion's (Status, Reason) -> SliceState.

e.g.

(Unknown, "") -> ACCEPTED (False, Incomplete) -> INITIALIZING (False, Activating) -> INITIALIZING ... (False, Failed) -> ERROR

pajakd added 2 commits November 5, 2025 12:13

Rename states + add timeout

6843811

Cleanup

ed3cdda

pajakd requested review from PBundyra and gabesaba as code owners November 5, 2025 12:47

pajakd added 2 commits November 5, 2025 12:56

Update api

b7ddc1d

Remove special logic for deformed/deactivating

c4bd76b

mbobrovskyi reviewed Nov 7, 2025

View reviewed changes

slice/internal/controller/workload_controller.go Outdated Show resolved Hide resolved

mbobrovskyi reviewed Nov 7, 2025

View reviewed changes

slice/internal/core/constants.go

package core

type MMIGHealthStatus string

Copy link

Collaborator

mbobrovskyi Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also move it to core.go.

mbobrovskyi reviewed Nov 7, 2025

View reviewed changes

slice/internal/core/slice.go Outdated Show resolved Hide resolved

mbobrovskyi reviewed Nov 7, 2025

View reviewed changes

slice/internal/util/testing/wrappers.go Outdated Show resolved Hide resolved

mbobrovskyi reviewed Nov 7, 2025

View reviewed changes

slice/internal/util/testing/wrappers.go Outdated Show resolved Hide resolved

mbobrovskyi reviewed Nov 7, 2025

View reviewed changes

slice/internal/core/slice.go Outdated Show resolved Hide resolved

mbobrovskyi reviewed Nov 7, 2025

View reviewed changes

pajakd and others added 6 commits November 7, 2025 13:27

Update slice/internal/util/testing/wrappers.go

074a33a

Co-authored-by: Mykhailo Bobrovskyi <[email protected]>

Update slice/internal/util/testing/wrappers.go

890f1bc

Co-authored-by: Mykhailo Bobrovskyi <[email protected]>

Update slice/internal/core/slice.go

6f6093d

Co-authored-by: Mykhailo Bobrovskyi <[email protected]>

Update slice/internal/controller/workload_controller.go

8e7ca0a

Co-authored-by: Mykhailo Bobrovskyi <[email protected]>

Update slice/internal/core/slice.go

55f4c9c

Co-authored-by: Mykhailo Bobrovskyi <[email protected]>

Address comments

bf39be9

gabesaba reviewed Nov 7, 2025

View reviewed changes

	type Type string
	// +kubebuilder:validation:Enum=v6e;tpu-v7x
	type Type string

		TPUSliceHealthNodeSelectorValue = "true"

		AcceleratorTpu7x = "tpu-v7x"

	cond := meta.FindStatusCondition(slice.Status.Conditions, string(v1alpha1.SliceStateConditionType))
	cond := meta.FindStatusCondition(slice.Status.Conditions, v1alpha1.SliceStateConditionType)

	func (s SliceWrapper) Type(Type string) SliceWrapper {
	func (s SliceWrapper) Type(acceleratorType string) SliceWrapper {

	func (s SliceWrapper) Type(Type string) SliceWrapper {
	func (s SliceWrapper) Type(t string) SliceWrapper {

	Type: string(v1alpha1.SliceStateConditionType),
	Type: v1alpha1.SliceStateConditionType,

	TypeTpu7x Type = "tpu-v7x"
	AcceleratorTypeTpu7x Type = "tpu-v7x"

	// Type specifies the type of accelerator used in this slice, e.g., "v6e", "tpu-v7x".
	// type specifies the type of accelerator used in this slice, e.g., "v6e", "tpu-v7x".

	// Topology represents the network topology of the slice.
	// topology represents the network topology of the slice.

	// Partition Ids denotes the set of partitions to use to form a slice
	// partitionIds denotes the set of partitions to use to form a slice

[slice] Rename states + add timeout #810

Are you sure you want to change the base?

[slice] Rename states + add timeout #810

Uh oh!

Conversation

pajakd commented Nov 5, 2025

Description

Issue

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbobrovskyi Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbobrovskyi Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbobrovskyi Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PBundyra commented Nov 7, 2025

Uh oh!

gabesaba left a comment

Choose a reason for hiding this comment

Uh oh!

mbobrovskyi Nov 7, 2025 •

edited

Loading

mbobrovskyi Nov 7, 2025 •

edited

Loading

mbobrovskyi Nov 7, 2025 •

edited

Loading